feat(mcp): support multiple active index bindings (RAAE-1604)#629
Open
vishal-bala wants to merge 6 commits into
Open
feat(mcp): support multiple active index bindings (RAAE-1604)#629vishal-bala wants to merge 6 commits into
vishal-bala wants to merge 6 commits into
Conversation
Treat the MCP `indexes` config as a real multi-binding map instead of enforcing exactly one logical binding. This is the foundation for multi-index MCP support (RAAE-1603); single-index configs and callers behave identically. Config: - Drop the "exactly one configured index binding" validator; require at least one binding with non-blank ids. - Add per-binding `description` and `read_only` fields. - Move schema inspection / runtime-mapping / search validation methods onto MCPIndexBindingConfig and remove the single-binding convenience accessors from MCPConfig. Runtime/server: - Introduce an immutable BindingRuntime bundling a binding's config, index, effective schema, vectorizer, native-hybrid support, and effective read-only flag. - Replace single-resource server state with a dict of BindingRuntime; startup inspects/validates/initializes every binding independently (one client per binding) with all-or-nothing teardown. - Resolve native-hybrid support eagerly per binding. - Size the concurrency semaphore from the max binding limit and pass a per-binding request timeout into run_guarded. - Add resolve_binding(index_id): defaults to the sole binding, raises invalid_request for omitted-on-multi and unknown ids. - Compute effective_read_only as global read-only OR per-binding read_only. Tools: - Re-thread search/upsert onto a resolved BindingRuntime instead of single-binding server accessors. Tests follow TDD: multi-binding config/startup coverage, resolve_binding semantics, semaphore sizing, per-binding teardown, and backward-compatible single-index behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
This was referenced Jun 16, 2026
Both fixtures used `user_index_{worker_id}` / `v1_{worker_id}` as index
name and prefix, causing a collision when the two test modules run on the
same xdist worker. Switch to `redis_test_name` (which incorporates the
node hash) so every test gets a unique namespace, and add `drop=True` to
`create(overwrite=True)` so stale documents from an interrupted run are
removed rather than reused.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Collaborator
Author
|
CI is failing because of flaky tests that will be fixed by redis-developer/sql-redis#39 |
nkanu17
reviewed
Jun 26, 2026
nkanu17
requested changes
Jun 26, 2026
Collaborator
There was a problem hiding this comment.
I really like the design!
Binding runtime dataclass is nice and like how resolve binding works as the single routing entry point shared across all tools keeps the behavior consistent and how it's backward compatible for single binding servers works
One blocking issue before merge:
the teardown loop in _teardown_runtime does not protect subsequent bindings if _close_resources raises on an earlier one.
- A failed disconnect on binding 1 will propagate out of the loop immediately, leaving bindings 2 through N with open Redis connections that are never closed.
- In a partial startup failure this is exactly the path that gets exercised, so the leak is not hypothetical.
- Wrapping each iteration in a try/except that logs and continues is a small fix and should be done before this lands.
nkanu17
reviewed
Jun 26, 2026
Three issues found in review: - upsert_records() never consulted rt.effective_read_only, so a binding configured with read_only: true could still accept writes when the global read_only flag was off. Add an explicit FORBIDDEN guard at the top of upsert_records() before any other processing. - _teardown_runtime() did not protect subsequent bindings if _close_resources() raised on an earlier one. A network error on binding 1 would propagate out of the loop, leaving bindings 2-N with open Redis connections. Wrap each iteration in try/except and log the failure so teardown is best-effort across all bindings. - _tools_registered was not reset in _teardown_runtime(), so a re-startup after a partial-startup failure would skip tool re-registration. Reset it alongside the other state clears. Also adds a clarifying comment on the semaphore size semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add regression tests for the behaviors introduced in 3a0d1fe, which shipped without coverage: - upsert-records rejects a write to a read-only binding with FORBIDDEN before any backend load is attempted. - _teardown_runtime continues closing the remaining bindings when one binding's close raises, and fully clears terminal state (including _tools_registered) so a later startup re-registers tools. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b6b2db1. Configure here.
Both convenience methods called resolve_binding(None), which raises invalid_request on any multi-index server — a latent footgun for tool authors that only surfaced at runtime under a multi-binding config. They had no non-test callers. Remove them and route all access through resolve_binding. Integration tests that used them now use small mcp_binding_index / mcp_binding_vectorizer test helpers in tests/conftest.py, which delegate to resolve_binding and preserve the same "vectorizer is not configured" / "has not been started" errors. Drop the now-dead mirror methods from the unit test fakes. Also align test_server_shutdown_disconnects_index_when_vectorizer_close_fails with the best-effort teardown introduced in 3a0d1fe: a failing vectorizer close is logged and swallowed (index still disconnected), so shutdown no longer re-raises it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1604) Stop resetting _tools_registered in _teardown_runtime. MCP tools are registered once on the FastMCP instance and their closures resolve the live binding at call time, so they survive teardown and stay valid across a stop/start. The lifecycle allows startup() again from STOPPED, and _register_tools() runs inside _initialize_runtime_resources, so resetting the flag caused a restart to re-register the same search-records/upsert-records names on the instance (duplicate/broken restart). The earlier "reset so a failed retry re-registers" concern does not occur: the flag is only set True after registration succeeds, and registrations persist on the instance regardless of binding teardown, so a retry that skips registration still has working tools. Update the teardown test to assert registration survives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The RedisVL MCP server currently binds to exactly one Redis index per process. That single-binding assumption is enforced by a config validator and baked throughout the codebase — single-resource server state, single-binding convenience accessors on
MCPConfig, and the search/upsert tools. Before the server can expose multiple logical indexes from a single endpoint (RAAE-1603), that assumption has to be removed and replaced with a real multi-binding model.This PR (RAAE-1604) does exactly that, and nothing more: it reshapes the configuration and runtime model so the server can start, inspect, validate, and serve one or many bindings, while keeping existing single-index configs and callers behaving identically. It is the foundation the rest of the epic (discovery via
list-indexes, index routing onsearch-records/upsert-records, docs) builds on, so it intentionally does not yet add any new request parameters or tools.Implementation
The core of the change is a new immutable
BindingRuntime(inredisvl/mcp/runtime.py) that bundles everything a tool call needs for one logical index: the binding config, the connectedAsyncSearchIndex, its effective (inspected + overridden) schema, an optional vectorizer, the resolved native-hybrid-search capability, and the effective read-only flag. The server now holds adict[str, BindingRuntime]keyed by logical id instead of a single set of_index/_vectorizerfields. Startup iterates every configured binding and inspects, validates, and initializes each one independently — each binding owns its own Redis client — with all-or-nothing teardown so a single bad binding fails startup cleanly without leaking connections.On the config side, the "exactly one configured index binding" validator is gone (we now simply require at least one binding with non-blank ids), and the schema-inspection, runtime-mapping, and search-validation methods move from
MCPConfigontoMCPIndexBindingConfigwhere they naturally belong per binding. The single-binding convenience accessors onMCPConfigare removed. Each binding gains optionaldescriptionandread_onlyfields, and a binding's effective write availability is computed as global--read-onlyOR the per-indexread_only. Tool resolution goes through a newserver.resolve_binding(index_id)helper that defaults to the sole binding when one is configured (preserving backward compatibility) and returns aninvalid_requesterror when an index is omitted with multiple bindings configured or when an unknown id is given. The search and upsert tools were re-threaded to operate on a resolvedBindingRuntimerather than reaching into single-binding server accessors.Additional notes:
BindingRuntime, replacing the previous lazy single-index cache.max_concurrencyacross bindings; the request timeout is sourced per-binding and passed explicitly intorun_guarded.get_index()/get_vectorizer()are retained as thin convenience wrappers overresolve_binding(None).description/read_onlydefaults,resolve_bindingrouting semantics, semaphore sizing, per-binding teardown, and three integration tests (multi-binding startup, global read-only override, and a single invalid binding failing startup), alongside the updated single-index tests that confirm backward compatibility.Verification
mypyclean across all source files;black/isortformatted.🤖 Generated with Claude Code
Note
Medium Risk
Touches MCP startup, Redis connection lifecycle, and tool execution paths;
run_guardedand removedget_index/get_vectorizerare breaking for subclasses and custom tools, though single-index YAML behavior is preserved via default binding resolution.Overview
Replaces the single-index MCP server with a multi-binding model:
indexesmay define one or many logical bindings (validator now requires at least one non-blank id, not exactly one).MCPConfigloses single-binding shortcuts; schema inspection, overrides, and search validation live onMCPIndexBindingConfig, which gains optionaldescriptionandread_only.Introduces immutable
BindingRuntimeand server statedict[str, BindingRuntime]. Startup initializes each binding independently (own Redis client, schema, vectorizer, native-hybrid probe); failures tear down already-built bindings. Newresolve_binding(index_id)defaults to the sole binding when one is configured; with multiple bindings, omitted or unknown ids returninvalid_request.get_index/get_vectorizerare removed from the server (tests useresolve_bindinghelpers).Search and upsert tools run against a resolved binding; upsert rejects
effective_read_only(global or per-binding).run_guardednow requires keywordtimeout_secondsper binding. Process-wide concurrency usesmax(max_concurrency)across bindings. Multi-binding setups omit schema-specific search tool hints until per-call index routing lands in a follow-up.Reviewed by Cursor Bugbot for commit cb9a12b. Bugbot is set up for automated code reviews on this repo. Configure here.
RedisVLMCPServer.run_guarded(operation_name, awaitable)now requires a keyword-onlytimeout_secondsargument (sourced from each binding'srequest_timeout_seconds). Any code that subclassesRedisVLMCPServeror callsrun_guardeddirectly (custom tools/plugins) must update its call sites to passtimeout_seconds=, otherwise it raisesTypeError: run_guarded() missing 1 required keyword-only argument: 'timeout_seconds'at call time. This repository has no CHANGELOG file, so this note serves as the migration callout (per review feedback on #629).